Add cla support#1026
Conversation
|
That looks good to me, feels like it's going the right way. |
|
Great, I'll keep going in that case. Currently wondering how to make a button actually update the CLA setting (and refresh the proposal). |
jt-nti
force-pushed
the
user-cla
branch
3 times, most recently
from
99a7835 to
98699ab
Compare
|
|
||
| # TODO this takes a while | ||
| # Can the proposal states be updated in the background? | ||
| @user.proposed.each do |pr| |
There was a problem hiding this comment.
@Floppy any idea how to make this happen in the background or is it not worth worrying about? (There's a noticeable delay, especially if you have lots of PRs open but that's unlikely in normal circumstances I guess.)
There was a problem hiding this comment.
Yeah, there's a background job runner that will do it. If you do UpdateProposalJob.perform_later pr.number I think that will work.
There was a problem hiding this comment.
Nice, that was a lot easier than I was expecting! Thanks.
There was a problem hiding this comment.
Helps that I had basically the exact same problem a while back :)
|
@Floppy I think it's mostly working now- probably needs some tests though!! |
jt-nti
force-pushed
the
user-cla
branch
4 times, most recently
from
a95b93d to
72389df
Compare
Floppy
left a comment
Floppy
left a comment
There was a problem hiding this comment.
Looks very nice. Made a few suggestions around DRYing up the code a little, but in general this looks great :)
| # Get activity list | ||
| presenter = ProposalPresenter.new(@proposal) | ||
| @activity = presenter.activity_log | ||
| # Does the proposer still need to sign the CLA? |
There was a problem hiding this comment.
As this is in both here and the edit controller, it feels like it could be pushed into the User model?
There was a problem hiding this comment.
That was bugging me as well, I'll move it
| set_build_status(:pending, I18n.t("build_status.cla.pending"), status) | ||
| end | ||
| else | ||
| set_build_status(:success, I18n.t("build_status.cla.none"), status) |
There was a problem hiding this comment.
Do we need to set the build status at all if there's no CLA? Probably not...
There was a problem hiding this comment.
Good idea, I hadn't thought of just not adding it, doh!
| return nil if pr_closed? | ||
| return "dead" if too_old? | ||
| return "blocked" if blocked? | ||
| return "blocked" if blocked? || (cla_required? && !cla_accepted?) |
There was a problem hiding this comment.
This logic happens quite a lot. I feel like there's a combined method like User#needs_to_sign_cla? that would reduce the places where both have to be checked.
jt-nti
force-pushed
the
user-cla
branch
2 times, most recently
from
e651835 to
6b3b7be
Compare
Signed-off-by: James Taylor <jt-git@nti.me.uk>
2 participants
Signed-off-by: James Taylor jt-git@nti.me.uk